Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: crop box overflows in "viewMode" 1 and 2 #381

Merged

Conversation

ingmarh
Copy link
Contributor

@ingmarh ingmarh commented Jun 24, 2018

PR Checklist

  • The commit message follows our guidelines.
  • Tests for the changes have been added (for bug fixes / features)
  • [n/a] Docs have been added / updated (for bug fixes / features)

PR Type

[x] Bugfix
[ ] Feature
[ ] Code style update
[ ] Refactor
[ ] Build related changes
[ ] Documentation content changes
[ ] Other, Please describe:

What is the current behavior?

When using "viewMode" 1 or 2, the crop box could overflow if the canvas is moved partly beyond the container boundaries.

If moved beyond the top/left boundaries, it could overflow the container. If moved beyond the bottom/right boundaries, it could overflow the canvas (exceeding its size).

This behavior can be reproduced on the demo page:
https://fengyuanchen.github.io/cropperjs/

Steps to reproduce:

  1. In "viewMode" 1 or 2 drag mode, switch to using "free" aspect ratio
  2. Drag the image beyond the top/left or bottom/right container boundaries
  3. Use "setData" with width/height values greater than the image
    (e.g. {"width":2000,"height":3000})

I could also reproduce it in aspect ratio mode, for example:

  1. In "viewMode" 1 or 2, switch to using "4:3" aspect ratio
  2. Drag the image beyond the top/left container boundaries
  3. Move the crop box to the top left position
  4. Use the "se" drag handle and drag it down

What is the new behavior?

In limited "viewMode", if the image canvas is moved beyond the boundaries of the container, it is prevented to inadvertently have the crop box overflow the boundaries of the container or image canvas when using the crop box drag handles or setData.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

First of all, thank you for Cropper.js!

We ran into this problem when testing our use case where we have input fields to set the desired width/height of the image. The code changes in this PR (tested carefully) should fix it.

@ingmarh ingmarh force-pushed the fix/crop-box-overflows-container branch from 8967e42 to 121b629 Compare June 25, 2018 07:42
@ingmarh ingmarh changed the title fix: crop box overflows in "viewMode" 1 fix: crop box overflows in "viewMode" 1 and 2 Jun 25, 2018
When using "viewMode" 1 or 2, the crop box could overflow if the canvas
is moved partly beyond the container boundaries.

If moved beyond the top/left boundaries, it could overflow the
container. If moved beyond the bottom/right boundaries, it could
overflow the canvas (exceeding its size).
@ingmarh ingmarh force-pushed the fix/crop-box-overflows-container branch 2 times, most recently from 51d5622 to 54e6dbe Compare June 26, 2018 17:41
@fengyuanchen
Copy link
Owner

fengyuanchen commented Jun 27, 2018

The crop box overflows container? I cannot reappear it, or may be I don't understand what you are meaning.

The crop box outline overflows container in vision is right and it is designed as this.

@ingmarh
Copy link
Contributor Author

ingmarh commented Jun 27, 2018

Yes. In view mode 1 and 2, if the canvas is moved/positioned beyond the container boundaries, the crop box can overflow the container and also the canvas (depending on which side out of the container the canvas was moved).

I can reproduce it on the Cropper.js demo page by following the steps in the PR description. Have you tried these?

I've tried to describe the issue as clear as possible in the description and in the code (tests), but I can also post a screenshot of the bug after the next week. (I'm on vacation right now with no access to a desktop computer.)

If I understood your quote correctly: It has nothing to do with the outline of the crop box. I know this is designed to be visually over the container borders and makes perfect sense.

Edit – More from code perspective: The maximum crop box size was set to be the canvas size (in limited mode). If part of the canvas is moved beyond the container boundaries, the offset was not deducted when calculating crop box max height/width.

@fengyuanchen
Copy link
Owner

Got it! Thanks!

@fengyuanchen fengyuanchen merged commit 44f50a3 into fengyuanchen:master Jun 30, 2018
fengyuanchen added a commit that referenced this pull request Jun 30, 2018
@ingmarh
Copy link
Contributor Author

ingmarh commented Jul 1, 2018

@fengyuanchen Thanks for merging.

But I realized that with the commit after the merge (086a568), half of the fix was reverted. That is, in the case when the canvas is dragged/moved beyond the container boundaries on the bottom and/or right sides, the crop box can overflow the canvas in limited view mode (1 and 2). See also steps to reproduce from the PR description.

I wrote a test for this case, but that was removed with e1b015b, unfortunately.

Although I wished it would be simpler, using the code in "render.js" from this PR will fix this case. You're right that it could be two Math.min less, though – so:

let maxCropBoxWidth = containerData.width;
let maxCropBoxHeight = containerData.height;

if (limited) {
  const canvasRight = containerData.width - canvasData.left - canvasData.width;
  const canvasBottom = containerData.height - canvasData.top - canvasData.height;

  maxCropBoxWidth = Math.min(
    containerData.width,
    canvasData.width,
    canvasData.width + (canvasRight < 0 ? canvasRight : canvasData.left),
  );
  maxCropBoxHeight = Math.min(
    containerData.height,
    canvasData.height,
    canvasData.height + (canvasBottom < 0 ? canvasBottom : canvasData.top),
  );
}

I can post a screenshot of the issue after the next week. Thanks!

@fengyuanchen
Copy link
Owner

You are right, I will revert it soon.

fengyuanchen added a commit that referenced this pull request Jul 2, 2018
@ingmarh
Copy link
Contributor Author

ingmarh commented Jul 2, 2018

👍 Your calculation for this case is much simpler.

@ingmarh ingmarh deleted the fix/crop-box-overflows-container branch July 2, 2018 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants